Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

mdhtml: init at 0.2.2 #261671

Merged
merged 2 commits into from
Oct 21, 2023
Merged

mdhtml: init at 0.2.2 #261671

merged 2 commits into from
Oct 21, 2023

Conversation

tomkoid
Copy link
Contributor

@tomkoid tomkoid commented Oct 17, 2023 •
edited
Loading

Description of changes

mdhtml is a simple CLI Markdown to HTML converter with styling support.

https://codeberg.org/Tomkoid/mdhtml

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@tomkoid tomkoid changed the title mdhtml: init at 0.2.1 mdhtml: init at 0.2.2 Oct 18, 2023
h7x4
h7x4 reviewed Oct 19, 2023
View reviewed changes
Copy link
Member

@h7x4 h7x4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool stuff! Just a few nits

pkgs/tools/text/mdhtml/default.nix Outdated Show resolved Hide resolved
pkgs/tools/text/mdhtml/default.nix Outdated Show resolved Hide resolved
pkgs/tools/text/mdhtml/default.nix Outdated Show resolved Hide resolved
pkgs/tools/text/mdhtml/default.nix Show resolved Hide resolved
@h7x4
Copy link
Member

h7x4 commented Oct 19, 2023

Also, please rebase your commits to look like this:

maintainers: add tomkoid
mdhtml: init at 0.2.2

@h7x4
Copy link
Member

h7x4 commented Oct 20, 2023

I believe you have the commits in the wrong order. Other than that, it looks good to me.

I don't think it's obligatory just yet, but a lot of packages tend to go into the new by-name package location, instead of all-packages.nix. This will become obligatory at some point. You can try it out if you'd like: https://github.com/NixOS/nixpkgs/blob/master/pkgs/by-name/README.md

@Janik-Haag
Copy link
Member

I don't think it's obligatory just yet, but a lot of packages tend to go into the new by-name

You don't need to do this now, there will be a later pr that moves everything possible via a script so don't bother for this pr, when you do another init just put it into pkgs/by-name since you don't have to edit the all-packages.nix

@Janik-Haag
Copy link
Member

Result of nixpkgs-review pr 261671 run on x86_64-linux 1

1 package built:
  • mdhtml

Janik-Haag
Janik-Haag approved these changes Oct 21, 2023
View reviewed changes
Copy link
Member

@Janik-Haag Janik-Haag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Janik-Haag Janik-Haag requested a review from h7x4 October 21, 2023 18:10
@Janik-Haag Janik-Haag merged commit 12a0c89 into NixOS:master Oct 21, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@h7x4 h7x4 h7x4 approved these changes

@Janik-Haag Janik-Haag Janik-Haag approved these changes

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 12.approvals: 2 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tomkoid @h7x4 @Janik-Haag @delroth @NixOSInfra